Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instructions to run coverage #737

Merged
merged 31 commits into from
Jun 5, 2020
Merged

Add instructions to run coverage #737

merged 31 commits into from
Jun 5, 2020

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Jun 2, 2020

The PR adds instructions to run coverage builds. It uses the subset of ros2.repos file (by now at my own fork I'll update the URL as soon as I send the PR for it to ros2/ros2).

Status is draft pending on @maryaB-osr's and @chapulina review and final URL location.

j-rivero added 3 commits May 15, 2020 20:09
The goal is to provide users with a method to call ci_linux_coverage job
in a way that the Jenkins build can exercise all testing mechanisms
present in all the ROS2 core packages.

The set in this commit is still a work in progress and will be modified.
@j-rivero j-rivero marked this pull request as draft June 2, 2020 23:07
@j-rivero j-rivero requested a review from maryaB-osr June 2, 2020 23:08
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 2, 2020 23:08 Inactive
@j-rivero j-rivero requested a review from chapulina June 2, 2020 23:08
@chapulina chapulina requested a review from nuclearsandwich June 3, 2020 00:36
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Jose!

source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
j-rivero and others added 4 commits June 3, 2020 14:02
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 3, 2020 12:03 Inactive
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 3, 2020 14:14 Inactive
@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 4, 2020

In the case we want to add instructions to run the coverage calculator script, they are in the branch coverage_set_ros2_script

@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 4, 2020 14:29 Inactive
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 4, 2020 14:43 Inactive
@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 4, 2020

made some improvements on formatting, ready to go once we have the final URL for coverage.repos file.

@dirk-thomas
Copy link
Member

Instead of creating and maintaining another .repos file the set should be generated using rosinstall_generator.

@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 4, 2020

Instead of creating and maintaining another .repos file the set should be generated using rosinstall_generator.

That sounds reasonable to me. I use it with my set of proposed packages and found some issues:

The following unreleased packages/stacks will be ignored: test_cli, test_cli_remapping, test_communication, test_launch_ros, test_quality_of_service, test_rclcpp, test_security, test_tf2

Seems to work with released packages, how should I include the repositories for test_* packages?

I assume that we need to store the set of proposed packages for coverage in a place and generate it dynamically somehow? at running time in Jenkins? on changes on ros2.ros2 repository?

@dirk-thomas
Copy link
Member

Seems to work with released packages, how should I include the repositories for test_* packages?

If a repository is not part of the distribution.yaml file then rosinstall_generator can't fetch it. For system_tests it might be best to just add a source entry for it. Then I would assume rosinstall_generator should be able to fetch it with --repos or --upstream-development.

Co-authored-by: Marya Belanger <marya@openrobotics.org>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 5, 2020 00:08 Inactive
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 5, 2020 00:12 Inactive
Co-authored-by: Marya Belanger <marya@openrobotics.org>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 5, 2020 00:12 Inactive
j-rivero and others added 3 commits June 5, 2020 02:12
Co-authored-by: Marya Belanger <marya@openrobotics.org>
Co-authored-by: Marya Belanger <marya@openrobotics.org>
Co-authored-by: Marya Belanger <marya@openrobotics.org>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 5, 2020 00:13 Inactive
@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 5, 2020

Please double check some of the edits I made to make sure I didn't change any intended meaning

You did not change any meaning but fix a good bunch of errors, thanks @maryaB-osr

@j-rivero j-rivero requested review from dirk-thomas, maryaB-osr and chapulina and removed request for nuclearsandwich June 5, 2020 00:17
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text looks good, I'm just debugging the get_coverage_ros2_pkg.py script directly with @j-rivero

source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
Co-authored-by: Louise Poubel <louise@openrobotics.org>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-737 June 5, 2020 00:34 Inactive
Copy link
Contributor

@maryaB-osr maryaB-osr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a couple little things, but otherwise lgtm!

source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
source/Contributing/ROS-2-On-boarding-Guide.rst Outdated Show resolved Hide resolved
j-rivero and others added 4 commits June 5, 2020 02:40
Co-authored-by: Marya Belanger <marya@openrobotics.org>
Co-authored-by: Marya Belanger <marya@openrobotics.org>
Co-authored-by: Marya Belanger <marya@openrobotics.org>
Co-authored-by: Marya Belanger <marya@openrobotics.org>

* From the ci_linux_coverage Jenkins build copy the URL of the build
* Download the `get_coverage_ros2_pkg <https://github.com/j-rivero/ros2_coverage_jenkins_params/blob/coverage_checker/get_coverage_results/get_coverage_ros2_pkg.py>`__ script
* Execute the script: ``./get_coverage_ros2_pkg.py <jenkins_build_url> <ros2_package_name>`` (`README <https://github.com/j-rivero/ros2_coverage_jenkins_params/blob/coverage_checker/get_coverage_results/README.md>`__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really recommend "external" scripts like this which have never been reviewed and are not accessible to edit by anyone else in the team?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j-rivero is working on moving it to a more common location. Do you have a preference?

I reviewed the script while reviewing this PR.

Copy link
Member

@dirk-thomas dirk-thomas Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is working on moving it to a more common location. Do you have a preference?

I don't have a preference - just not on a personal GitHub user. Please suggest proposals.

I reviewed the script while reviewing this PR.

Just a view things I notices while briefly looking at it (and since I don't see any PR on that repo I will just post it here):

  • shebang line doesn't use /usr/bin/env
  • incorrect usage handling
  • error messages don't go to stderr
  • 7 line convoluted logic to determine a simple boolean
  • parentheses around conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a preference - just not on a personal GitHub user. Please suggest proposals.

Fully agree on moving it a better location. ros2/ci repository is the first one that comes to my mind.

Just a view things I notices while briefly looking at it (and since I don't see any PR on that repo I will just post it here):

Thanks for looking into it Dirk. I've updated the code with your findings but let's start a PR procedure to look into it deeper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants